-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Rockets endpoint #35
Implement Rockets endpoint #35
Conversation
***Why?*** - To retrieve rocket's info using the rockets API endpoint provided by SpaceX ***How?*** - Created a Rockets model that exposes an `info` method - This method accepts an optional `rocket_id` - When none passed, it returns the information of all the rockets - Otherwise returns info of the specified rocket id ***Side effects*** - This implementation only makes the info method public whereas other classes in the module are exposing all the methods. This is something that can be fixed in a separate PR
Pull Request Test Coverage Report for Build 60
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @invacuo You did a great job! Thank you very much!
end | ||
|
||
it "returns all Rockets' when no id is passed info" do | ||
first_subject = subject.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
get(rocket_id) | ||
end | ||
|
||
private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason some endpoints have this get
or retrieve_all
public is that at some point in the past these were exposed as the default methods. So in order to make them backwards compatible, even after we adopted the info
and info(foo)
as default, I decided to keep carrying them over. You'll probably see people opening PRs using get
, retrieve_all
instead of info
(which is understandable).
I saw your other comments regarding this and I agree with you specially because our release is still at minor versions 0.0.8
. Thats the reason I'm not documenting these method on README anymore. I'm planning to release a major version 1.0.0
after closing this list of PRs.
By the way, I got your twitter message. You're more than welcome to help out!
- `SPACEX::Rockets.info` Retrieve all Rockets; | ||
- `SPACEX::Rockets.info('falcon1')` Retrieve a specific rocket. Ex: `falcon1` | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for skipping the example like we did with all methods? Do you feel that it is an overhead adding examples for each method?
Why?
How?
info
methodrocket_id
Side effects/ notes
info
method public whereas other classes in the gem are exposing all the methods. This is something that I will fix in a separate PR